I guess we are good now that everything uses the caching ReflectHelper on current main?

(Except that we need to investigate if it's possible to simplify the implementation of reflection operations under .NET Framework)

Pull request overview

This PR addresses #3043 by consolidating reflection-related behavior around IReflectionOperations/ReflectionOperations, replacing several usages of ReflectHelper in PlatformServices (and updating tests accordingly) while introducing a small ReflectionHelper utility for test metadata extraction.

Changes:

• Extend IReflectionOperations and ReflectionOperations with cached/generic attribute APIs (e.g., GetAttributes<T>, GetCustomAttributesCached, IsAttributeDefined<T>).
• Update deployment + execution code paths to use IReflectionOperations instead of ReflectHelper in key locations.
• Refactor and expand unit/integration tests, adding MockableReflectionOperations to support Moq scenarios.

Copilot's findings

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx
PR: #7344 — Unify reflection helpers

Key Findings

This PR refactors tests to use the new unified ReflectionOperations/IReflectionOperations API (replacing ReflectHelper). The migration is structurally sound — the deleted ReflectHelperTests.cs tests have been faithfully ported into ReflectionOperationsTests.cs, and the new MockableReflectionOperations helper is well-designed. Tests correctly manage PlatformServiceProvider.Instance state via constructor setup and Dispose.

The following issues are all non-blocking cleanup items introduced during the migration:

1. [Assertion] 12 occurrences of SequenceEqual(...).Should().BeTrue() — These produce completely opaque failure messages on CI. Use actual.Should().Equal(expected) for sequence-order assertions (shown in inline comment at line 59 of ReflectionOperationsTests.cs).

2. [Structure] Stale test names HasAttributeDerivedFrom* — These methods now call rh.IsAttributeDefined<T>() internally (the old HasAttributeDerivedFrom method no longer exists). Names should be updated to reflect what's actually being tested.

3. [Structure] Two dead code comments — // new Mock<MemberInfo>(); at lines 258 and 305 should be removed.

4. [Assertion] Redundant Moq setup in TypeValidatorTests — Two identical It.IsAny<TypeInfo>() setups where the first (Returns(false)) is immediately overridden by the second (Returns(true)). Caused by a matcher change during migration from It.IsAny<Type>().

Recommendations

All issues are minor clean-up items that don't affect test correctness or reliability. No flakiness risks or false-positives were identified. The PR can proceed — the suggestions can be addressed in a follow-up or in this PR at the author's discretion.

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Summary

Workflow: pr-expert-reviewer 🧠
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

🔴 Correctness — Unsafe concrete-type casts (2 occurrences)

Both TestDeployment.cs:37 and DeploymentUtilityBase.cs:28 were changed to use the new IReflectionOperations abstraction — but both no-arg constructors still cast through to the concrete type:

new DeploymentItemUtility((ReflectionOperations)PlatformServiceProvider.Instance.ReflectionOperations)

DeploymentItemUtility now accepts IReflectionOperations, making the cast entirely unnecessary. It will throw InvalidCastException any time the active platform-service provider returns something other than the concrete ReflectionOperations (e.g., test scenarios using SetupMockReflectionOperations() / MockableReflectionOperations).

🟡 Correctness — DeclaringType! null-forgiving on a nullable property

IsMethodDeclaredInSameAssemblyAsType uses method.DeclaringType!.Assembly — the null-forgiving operator suppresses the compiler warning but does not prevent a NullReferenceException if a MethodInfo with DeclaringType == null is ever passed. A ?. guard with an appropriate fallback is the safer pattern.

Positive Observations

• The overall design is good: moving attribute caching directly into ReflectionOperations (away from ReflectHelper) is the right architectural move and significantly reduces duplication.
• ConcurrentDictionary<ICustomAttributeProvider, Attribute[]> for the attribute cache is thread-safe and well-suited for the parallel test-execution model.
• MockableReflectionOperations elegantly bridges the Moq generic-method-constraint gap and enables clean test isolation.
• The GetTestPropertiesAsTraits refactor (two-pass count + single allocation) is a nice performance improvement.
• Deletion of ReflectionOperationsNetFrameworkAttributeHelpers.cs (reflection-only load path) appears safe: code that still needs reflection-only load (AssemblyLoadWorker) already accesses CustomAttributeData.GetCustomAttributes directly and does not go through ReflectionOperations.

Generated by pr-expert-reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx
PR: #7344 — Unify reflection helpers (updated review, head 0253483)

Key Findings

This review covers the new commits since the previous automated review (52b4576 → 0253483). The migration from ReflectHelper to IReflectionOperations across the test suite is structurally sound. MockableReflectionOperations is well-designed and correctly bridges the Moq generic-constraint gap. TestablePlatformServiceProvider.SetupMockReflectionOperations wrapping is correct. The new GetTestCategories and IsAttributeDefined tests are well-isolated (state cleaned up via Dispose).

Four issues found — none are flakiness/isolation bugs, all are quality improvements:

1. [Structure] Dead SetupDeploymentItems method with pragma suppression in DesktopTestDeploymentTests.cs — the method is never called; a valid IDE0051 warning is suppressed rather than the dead code being removed or wired in.

2. [Assertion] AttributeMockingHelper.SetCustomAttribute ignores its Type parameter — the stored Type is never used in GetCustomAttributesNotCached, making the parameter misleading and creating a latent false-positive risk for future tests.

3. [Assertion] BeEquivalentTo used for ordered-sequence assertions in ReflectionUtilityTests.cs (lines 49, 63, 129) — where the test clearly expects a specific attribute order (derived before base), order-insensitive comparison masks regressions.

4. [Assertion] SequenceEqual(...).Should().BeTrue() (12 occurrences) — already flagged in the prior review; still unaddressed. Produces opaque CI failures.

Recommendations

• Remove or wire in SetupDeploymentItems in DesktopTestDeploymentTests.cs
• Either apply type-based filtering in GetCustomAttributesNotCached, or drop the Type type parameter from SetCustomAttribute
• Replace BeEquivalentTo with Equal for tests that encode a meaningful attribute order
• Replace SequenceEqual(...).Should().BeTrue() with actual.Should().Equal(expected) throughout

Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

🔴 Correctness — Two surviving concrete-type casts (unresolved from prior review)

TestDeployment.cs:37 and DeploymentUtilityBase.cs:28 both no-arg constructors still contain (ReflectionOperations)PlatformServiceProvider.Instance.ReflectionOperations. Since DeploymentItemUtility was updated to accept IReflectionOperations, the cast is entirely unnecessary — and actively harmful. It will throw InvalidCastException at runtime when the service provider resolves to anything other than the concrete ReflectionOperations (e.g., the MockableReflectionOperations now in the test project). The "Fix build errors" commit (latest, 0253483) did not address these.

🔴 Correctness — DeclaringType! null-forgiving in IsMethodDeclaredInSameAssemblyAsType (unresolved from prior review)

method.DeclaringType!.Assembly.Equals(type.Assembly) at line 287 of ReflectionOperations.cs silences the compiler but does not prevent a NullReferenceException. MethodInfo.DeclaringType is nullable by contract (returns null for global IL methods). A ?. guard returning false is the correct fix.

🟡 Correctness — Unguarded (Assembly) cast in NotCachedReflectionAccessor (new)

NotCachedReflectionAccessor.GetCustomAttributesNotCached (line 279) falls back to (Assembly)attributeProvider when the provider is not a MemberInfo. ICustomAttributeProvider is also implemented by Module and ParameterInfo — passing either silently throws InvalidCastException. A switch expression covering all known cases with a descriptive exception for the rest would prevent silent surprises.

Positive Observations

• Moving the attribute cache from the old ReflectHelper static singleton into ReflectionOperations is the right structural improvement; ConcurrentDictionary<ICustomAttributeProvider, Attribute[]> is thread-safe and handles parallel test execution correctly.
• The NotCachedReflectionAccessor inner class cleanly separates cached and uncached access paths, and the static Attribute[] GetAttributes(...) local function avoids a Func<> allocation on every cache-miss.
• GetTestPropertiesAsTraits's two-pass count + single allocation pattern is a genuine performance win.
• ClearCache() is correctly scoped to test manipulation scenarios and doesn't touch shared state.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

All three issues flagged in prior reviews have been correctly resolved in commit fae08031:

✅ Resolved — Unsafe (ReflectionOperations) casts removed

Both TestDeployment and DeploymentUtilityBase no-arg constructors now pass PlatformServiceProvider.Instance.ReflectionOperations directly to DeploymentItemUtility without a concrete-type cast. The InvalidCastException risk when using MockableReflectionOperations in tests is eliminated.

✅ Resolved — DeclaringType! null-forgiving operator replaced

IsMethodDeclaredInSameAssemblyAsType now correctly uses method.DeclaringType?.Assembly.Equals(type.Assembly) ?? false, guarding against global IL methods where DeclaringType is null by contract.

✅ Resolved — NotCachedReflectionAccessor hard-cast replaced

The (Assembly)attributeProvider cast is replaced with a well-structured switch expression that explicitly handles MemberInfo and Assembly cases, and throws a descriptive ArgumentException (which is caught and logged upstream) for unsupported providers like Module or ParameterInfo.

Positive Observations

• The switch expression in GetCustomAttributesNotCached is clean and correctly handles all framework-significant cases; the _ => throw new ArgumentException(...) arm produces a descriptive diagnostic rather than a silent miscast.
• The ?. null-conditional on DeclaringType with a ?? false fallback is exactly the right pattern.
• No new correctness, threading, or API-compatibility concerns were introduced in this commit.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx
PR: #7344 — Unify reflection helpers (updated review, head fae0803)

What Was Fixed in This Commit ✅

This commit ("Address review comments") resolves the four issues flagged in the prior review:

• All 12 SequenceEqual(...).Should().BeTrue() occurrences in ReflectionOperationsTests.cs → replaced with actual.Should().Equal(expected) ✅
• 3× BeEquivalentTo in ReflectionUtilityTests.cs (integration) → replaced with Equal ✅
• Dead SetupDeploymentItems method in DesktopTestDeploymentTests.cs → removed ✅
• MockableReflectionOperations.GetCustomAttributesCached → null-safe for the Assembly branch ✅
• AssemblyEnumeratorTests.TestableAssemblyEnumerator → simplified to Mock<ReflectHelper> directly ✅

Remaining Issues

Two items from the earlier reviews are still open:

1. [Assertion] ReflectionUtilityTests.cs:30 — One SequenceEqual(...).Should().BeTrue() was missed because it lives inside a #if NETFRAMEWORK block. (Inline comment posted.)

2. [Assertion] AttributeMockingHelper.SetCustomAttribute — the Type type parameter is stored but never used during filtering in GetCustomAttributesNotCached. The actual type filtering happens upstream via .OfType<T>() in MockableReflectionOperations, so there's no false-positive today, but the API is misleading. (Inline comment posted.)

Two items from the first review (run 25663966959) remain open but are minor/style-level:

3. Stale test method names HasAttributeDerivedFrom* (lines 272, 285, 298, 319 of ReflectionOperationsTests.cs) — these methods now call IsAttributeDefined<T> internally.
4. Dead code comments // new Mock<MemberInfo>(); (lines 258, 305 of ReflectionOperationsTests.cs).

Overall Assessment

No flakiness or isolation bugs. The migration from ReflectHelper to IReflectionOperations is sound and well-tested. All remaining items are minor clean-up suggestions.

🧪 Test quality reviewed by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Copilot's findings

• Files reviewed: 25/25 changed files
• Comments generated: 0 new

Copilot's findings

        // Setup mocks
        _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetCustomAttributes(methodInfo)).Returns(attributes);
        _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetAttributes<DataSourceAttribute>(methodInfo)).Returns(attributes.OfType<DataSourceAttribute>());

        // Setup mocks
        _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetCustomAttributes(_methodInfo)).Returns(attributes);
        _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetAttributes<DataSourceAttribute>(_methodInfo)).Returns([dataSourceAttribute]);

        // Setup mocks
        _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetCustomAttributes(_methodInfo)).Returns(attributes);
        _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetCustomAttributesCached(_methodInfo)).Returns(attributes);

        // Setup mocks
        _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetCustomAttributes(_methodInfo)).Returns(attributes);
        _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetCustomAttributesCached(_methodInfo)).Returns(attributes);

• Files reviewed: 26/26 changed files
• Comments generated: 6

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

1. Dead mock setups in TestMethodRunnerTests — Six new mock setups (GetAttributes<DataSourceAttribute> ×3, GetCustomAttributesCached ×3) are never actually called. MockableReflectionOperations routes both methods through mock.GetCustomAttributes, not through the respective generic/cached mock methods. The tests pass correctly via the existing GetCustomAttributes setup, but the extra setups are misleading and create maintenance confusion.

2. MockableReflectionOperations.GetCustomAttributesCached doesn't cache — While this is an acceptable tradeoff for a test double, the name implies once-per-member semantics. Without a comment documenting this intentional difference, future readers may assume the wrapper has caching behavior equivalent to the production ReflectionOperations.

3. Missing coverage for GetSingleAttributeOrDefault and GetFirstAttributeOrDefault — Both new IReflectionOperations methods lack unit tests. In particular, GetSingleAttributeOrDefault's InvalidOperationException throw path and GetFirstAttributeOrDefault's sealed-type assertion are untested.

Positive Observations

• The migration from SequenceEqual(...).Should().BeTrue() to actual.Should().Equal(expected) throughout the test suite is a meaningful improvement in assertion quality.
• The MockableReflectionOperations design cleanly solves the Moq/generic-method-with-constraints limitation while keeping test setup intuitive.
• The IsAttributeDefinedShouldReturnFromCache / HasAttributeDerivedFromShouldReturnFromCache tests correctly validate caching behavior via the PlatformServiceProvider.Instance indirection and are architecturally sound.
• Integration tests properly migrated from ReflectionOnlyLoadFrom to Assembly.LoadFrom, removing the fragile ReflectionOnlyAssemblyResolve handler.

Recommendations

• Remove the 6 redundant mock setups in TestMethodRunnerTests.
• Add a clarifying comment to MockableReflectionOperations.GetCustomAttributesCached about the intentional absence of caching.
• Add unit tests for GetSingleAttributeOrDefault (including the multi-attribute exception path) and GetFirstAttributeOrDefault.

Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

Performance Regression — GetRetryAttribute allocates an iterator state machine (Medium)

TestMethodInfo.GetRetryAttribute() now calls GetAttributes<RetryBaseAttribute>(MethodInfo), which is implemented as a yield return iterator. The old code explicitly avoided this allocation with a comment: "Iterate the cached attribute array directly to avoid allocating the yield-return state machine." Since GetRetryAttribute is called from the TestMethodInfo constructor (once per executed test method), this reintroduces measurable GC pressure at scale. See inline comment.

Defensive Coding Gap — NotCachedReflectionAccessor NullReferenceException + undocumented contract (Low-Medium)

The _ => arm in GetCustomAttributesNotCached evaluates attributeProvider.GetType() in the exception message before guarding against null, so a null input surfaces as NullReferenceException rather than a clean ArgumentNullException. Additionally, valid ICustomAttributeProvider subtypes (ParameterInfo, Module) throw ArgumentException at runtime without the interface contract documenting this constraint. See inline comment.

Positive Observations

• The consolidation of the attribute cache from ReflectHelper (singleton) into ReflectionOperations is architecturally sound — ConcurrentDictionary<ICustomAttributeProvider, Attribute[]> is the right data structure for thread-safe, low-memory-footprint attribute caching.
• Deletion of ReflectionOperationsNetFrameworkAttributeHelpers is justified: AssemblyLoadWorker uses CustomAttributeData directly for reflection-only assemblies, so there are no product code paths that require the old reflection-only-load workaround.
• GetRetryAttribute's two-step enumerator pattern (get first, check for second) correctly preserves the single-attribute invariant and properly disposes the enumerator via using.
• MockableReflectionOperations is a clean solution to the Moq generic-method-with-constraint limitation.
• GetTestPropertiesAsTraits's two-pass (count then fill) approach correctly avoids a List<T> allocation for the common zero-traits case.
• InitializeLifetimeService is correctly guarded with #if NET5_0_OR_GREATER [Obsolete] to suppress the BCL's obsolete warning while preserving the infinite-lifetime behavior on .NET Framework.

Recommendations

1. Restore the direct cache-array iteration in GetRetryAttribute to avoid the state-machine allocation.
2. Add a null guard and a clear exception message in NotCachedReflectionAccessor.GetCustomAttributesNotCached.

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠